8387488: [lworld] C2: acmp expansion fails with assert(replace != nullptr) failed: must succeed by TobiHartmann · Pull Request #2602 · openjdk/valhalla

TobiHartmann · 2026-06-30T13:46:33Z

When slightly modifying the test that I added for JDK-8386067, I hit an assert in CallStaticJavaNode::replace_is_substitutable because expansion of the substitutability call failed and emit_substitutability_check returned nullptr although can_emit_substitutability_check returned true.

In the new test, the problematic field is ObjectHolder::obj which, although it is of type Object, is always exact and therefore can't be a value object and does not require a substitutability test. InlineTypeNode::can_emit_substitutability_check correctly returns true:

valhalla/src/hotspot/share/opto/inlinetypenode.cpp

Lines 611 to 614 in 2ae8e47

    
           if (!lhs->bottom_type()->is_ptr()->can_be_inline_type() || 
        
               (rhs != nullptr && !rhs->bottom_type()->is_ptr()->can_be_inline_type())) { 
        
             return true; 
        
           }

However, during recursive expansion, nullable value object fields are null-checked and then compared field-by-field:

valhalla/src/hotspot/share/opto/inlinetypenode.cpp

Line 910 in 2ae8e47

    
           cur_lhs_field = kit->null_check_common(cur_lhs_field, T_OBJECT, false, &null_unknown);

The code unconditionally rebuilt those fields with InlineTypeNode::make_from_oop after the null and type checks because there's a CastPP in-between:

valhalla/src/hotspot/share/opto/inlinetypenode.cpp

Lines 955 to 960 in 2ae8e47

    
           cur_lhs_field = kit->gen_checkcast(cur_lhs_field, vk_klass, &not_vk); 
        
           if (!not_vk->is_top()) { 
        
             region->add_req(not_vk); 
        
             result->add_req(igvn.intcon(0)); 
        
           } 
        
           cur_lhs_field = InlineTypeNode::make_from_oop(kit, cur_lhs_field, vk);

If the field was already scalarized, this discarded the existing InlineTypeNode and reloaded fields from the oop, losing precise information such as exact Object field Phis. The later Object field comparison then looks non-exact and emit_substitutability_check_pointer returns nullptr:

valhalla/src/hotspot/share/opto/inlinetypenode.cpp

Lines 722 to 724 in 2ae8e47

    
           if (!lhs_type->is_inlinetypeptr() && !rhs_type->is_inlinetypeptr()) { 
        
             return nullptr; 
        
           }

The fix is to preserve already-scalarized recursive field operands when their inline klass matches the comparison klass, and only checkcast/reload operands that are not scalarized. With the fix, exact type information is preserved and we emit a pointer comparison for the exact object fields:

valhalla/src/hotspot/share/opto/inlinetypenode.cpp

Lines 700 to 702 in 2ae8e47

    
           } else if (!lhs_type->is_ptr()->can_be_inline_type() || !rhs_type->is_ptr()->can_be_inline_type()) { 
        
             // If one of the sides is not a value object, can only be substitutable if they are the same 
        
             cmp = kit->CmpP(lhs, rhs);

The IR Framework test verifies this successful expansion.

Note: The null and type checks are not immediately folded because by GVN because we need to set set_delay_transform here:

valhalla/src/hotspot/share/opto/callnode.cpp

Lines 1412 to 1416 in 2ae8e47

    
           igvn->set_delay_transform(true); 
        
           GraphKit kit(this, *igvn); 
        
           Node* replace = InlineTypeNode::emit_substitutability_check(&kit, left, right); 
        
           igvn->set_delay_transform(false);

Thanks,
Tobias

I confirm that I make this contribution in accordance with the OpenJDK Interim AI Policy.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed (1 review required, with at least 1 Committer)

Issue

JDK-8387488: [lworld] C2: acmp expansion fails with assert(replace != nullptr) failed: must succeed (Bug - P4)

Reviewers

Quan Anh Mai (@merykitty - Committer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/valhalla.git pull/2602/head:pull/2602
$ git checkout pull/2602

Update a local copy of the PR:
$ git checkout pull/2602
$ git pull https://git.openjdk.org/valhalla.git pull/2602/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 2602

View PR using the GUI difftool:
$ git pr show -t 2602

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/valhalla/pull/2602.diff

Using Webrev

Link to Webrev Comment

bridgekeeper · 2026-06-30T13:47:30Z

👋 Welcome back thartmann! A progress list of the required criteria for merging this PR into lworld will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2026-06-30T13:48:00Z

@TobiHartmann This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8387488: [lworld] C2: acmp expansion fails with assert(replace != nullptr) failed: must succeed

Reviewed-by: qamai

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 4 new commits pushed to the lworld branch:

285c9fc: 8387564: [lworld] cherry-pick JDK-8387554 ProblemList vmTestbase/nsk/jvmti/unit/functions/Dispose/JvmtiTest/TestDescription.java in virtual thread configs
50d7001: 8386719: [lworld] Remaining ValueClass argument verification issues
71eb703: 8386160: [lworld] Post jdk-27+25 compiler/valhalla/inlinetypes/TestIntrinsics.java#id4 assertion
... and 1 more: https://git.openjdk.org/valhalla/compare/2ae8e47da016bb4d7718e7923c964c2b91c638e6...lworld

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the lworld branch, type /integrate in a new comment.

TobiHartmann · 2026-06-30T14:21:23Z

        return v;
    }

+    // TODO 8376254: C1 bails out if the type of the nullable flat field is uninitialized


Just found this missing TODO when browsing code.

mlbridge · 2026-06-30T14:27:32Z

Webrevs

merykitty · 2026-06-30T14:46:22Z

-      if (!not_vk->is_top()) {
-        region->add_req(not_vk);
-        result->add_req(igvn.intcon(0));
+      if (cur_lhs_inline != nullptr && cur_lhs_inline->inline_klass() == vk) {


You can add a comment that it is not an issue if cur_lhs_inline is nullable and cur_lhs_field is null-checked, it is null-checked so that we can load the fields, and an InlineTypeNode does that already.

It's not quite what I want to say. The null check is still present, the thing is that we ignored the null-checked value (the CastPP) and just use the raw value. This is because normally, we need a non-null CastPP to load from the object. That is unnecessary for an InlineTypeNode because we don't perform the loads.

Okay, here's a new attempt 🙂

merykitty · 2026-06-30T14:47:10Z

    }
+
+    // TODO 8376254: C1 bails out if the type of the nullable flat field is uninitialized
+    static ObjectHolderHolder tmp = new ObjectHolderHolder();


It would probably be better to name this more specific to its type name so as not to have name collision.

marc-chevalier

Haven't found anything crazy, but with low confidence. Happy @merykitty was faster than me at reviewing this.

TobiHartmann · 2026-06-30T15:10:12Z

Thanks for the reviews Quan Anh and Marc! @merykitty I added a comment and adjusted the field name in the test. Please let me know what you think.

merykitty

Thanks a lot, it looks good to me.

TobiHartmann · 2026-07-01T07:35:30Z

Thanks for the review Quan Anh!

TobiHartmann · 2026-07-01T07:35:36Z

/integrate

openjdk · 2026-07-01T07:36:38Z

Going to push as commit 43153f5.
Since your change was applied there have been 4 commits pushed to the lworld branch:

285c9fc: 8387564: [lworld] cherry-pick JDK-8387554 ProblemList vmTestbase/nsk/jvmti/unit/functions/Dispose/JvmtiTest/TestDescription.java in virtual thread configs
50d7001: 8386719: [lworld] Remaining ValueClass argument verification issues
71eb703: 8386160: [lworld] Post jdk-27+25 compiler/valhalla/inlinetypes/TestIntrinsics.java#id4 assertion
... and 1 more: https://git.openjdk.org/valhalla/compare/2ae8e47da016bb4d7718e7923c964c2b91c638e6...lworld

Your commit was automatically rebased without conflicts.

openjdk · 2026-07-01T07:36:48Z

@TobiHartmann Pushed as commit 43153f5.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Prototype

828c4c0

TobiHartmann changed the title ~~Prototype~~ 8387488: [lworld] C2: acmp expansion fails with assert(replace != nullptr) failed: must succeed Jun 30, 2026

TobiHartmann added 2 commits June 30, 2026 16:10

Workaround for C1 issue

d0f66d9

Found another missing TODO

6adb893

TobiHartmann marked this pull request as ready for review June 30, 2026 14:21

TobiHartmann commented Jun 30, 2026

View reviewed changes

openjdk Bot added the rfr Pull request is ready for review label Jun 30, 2026

merykitty reviewed Jun 30, 2026

View reviewed changes

marc-chevalier reviewed Jun 30, 2026

View reviewed changes

Review comments

1016b74

New attempt at comment

65d1515

merykitty approved these changes Jul 1, 2026

View reviewed changes

openjdk Bot added the ready Pull request is ready to be integrated label Jul 1, 2026

openjdk Bot added the integrated Pull request has been integrated label Jul 1, 2026

openjdk Bot closed this Jul 1, 2026

openjdk Bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jul 1, 2026

	if (!lhs->bottom_type()->is_ptr()->can_be_inline_type() \|\|
	(rhs != nullptr && !rhs->bottom_type()->is_ptr()->can_be_inline_type())) {
	return true;
	}

	cur_lhs_field = kit->gen_checkcast(cur_lhs_field, vk_klass, &not_vk);
	if (!not_vk->is_top()) {
	region->add_req(not_vk);
	result->add_req(igvn.intcon(0));
	}
	cur_lhs_field = InlineTypeNode::make_from_oop(kit, cur_lhs_field, vk);

	if (!lhs_type->is_inlinetypeptr() && !rhs_type->is_inlinetypeptr()) {
	return nullptr;
	}

	} else if (!lhs_type->is_ptr()->can_be_inline_type() \|\| !rhs_type->is_ptr()->can_be_inline_type()) {
	// If one of the sides is not a value object, can only be substitutable if they are the same
	cmp = kit->CmpP(lhs, rhs);

	igvn->set_delay_transform(true);
	GraphKit kit(this, *igvn);

	Node* replace = InlineTypeNode::emit_substitutability_check(&kit, left, right);
	igvn->set_delay_transform(false);

Uh oh!

Conversation

TobiHartmann commented Jun 30, 2026 • edited by openjdk Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper Bot commented Jun 30, 2026

Uh oh!

openjdk Bot commented Jun 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TobiHartmann Jun 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mlbridge Bot commented Jun 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

merykitty Jun 30, 2026

Choose a reason for hiding this comment

Uh oh!

merykitty Jun 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TobiHartmann Jul 1, 2026

Choose a reason for hiding this comment

Uh oh!

merykitty Jun 30, 2026

Choose a reason for hiding this comment

Uh oh!

marc-chevalier left a comment

Choose a reason for hiding this comment

Uh oh!

TobiHartmann commented Jun 30, 2026

Uh oh!

merykitty left a comment

Choose a reason for hiding this comment

Uh oh!

TobiHartmann commented Jul 1, 2026

Uh oh!

TobiHartmann commented Jul 1, 2026

Uh oh!

openjdk Bot commented Jul 1, 2026

Uh oh!

openjdk Bot commented Jul 1, 2026

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

TobiHartmann commented Jun 30, 2026 •

edited by openjdk Bot

Loading

openjdk Bot commented Jun 30, 2026 •

edited

Loading

TobiHartmann Jun 30, 2026 •

edited

Loading

mlbridge Bot commented Jun 30, 2026 •

edited

Loading

merykitty Jun 30, 2026 •

edited

Loading